Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Diesels MultiConnections Derive #3796

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BlackDex
Copy link
Collaborator

@BlackDex BlackDex commented Aug 24, 2023

With this PR we remove almost all custom macro's to create the multiple database type code. This is now handled by Diesel it self.

This removed the need of the following functions/macro's:

  • db_object!
  • ::to_db
  • .from_db()

It is also possible to just use one schema instead of multiple per type.


NOTE: There are just few items which cause some issues with models and schemas having the exact same name, and that causes some ambiguity. I have solved that currently by adding schema:: in-front of those specific items. But it would be nicer if we didn't need that though.

Not sure what the best way is to address this. Probably renaming the tables is the best option i think.

I still want to do some testing with the different database types with an empty database, an existing database which needs migrations etc.. etc..

Also, we probably want to wait until Diesel releases v2.1.1, since there are currently some bugs and missing features which got fixed, but not yet merged.

And if someone else wants to help test, please do!, The more the better.

@BlackDex BlackDex force-pushed the diesel-multiconnect branch 5 times, most recently from 467af69 to 4780749 Compare September 1, 2023 07:30
@BlackDex BlackDex force-pushed the diesel-multiconnect branch from 4780749 to 2d09eba Compare September 2, 2023 22:23
@BlackDex BlackDex force-pushed the diesel-multiconnect branch from 2d09eba to 2c99858 Compare October 7, 2023 14:30
@BlackDex BlackDex force-pushed the diesel-multiconnect branch from 2c99858 to a35b68a Compare October 21, 2023 12:38
With this PR we remove allmost all custom macro's to create the multiple
database type code. This is now handled by Diesel it self.

This removed the need of the following functions/macro's:
- `db_object!`
- `::to_db`
- `.from_db()`

All `conn` variables do not need a `mut` anymore (says nightly rust)

It is also possible to just use one schema instead of multiple per type.
@BlackDex BlackDex force-pushed the diesel-multiconnect branch from a35b68a to 1b1596c Compare October 21, 2023 21:39
@weiznich
Copy link

Is there anything from the diesel side of things I can to do move this forward?

@BlackDex
Copy link
Collaborator Author

Is there anything from the diesel side of things I can to do move this forward?

Not really, but thanks for asking.

We don't want to break a pending PR for SSO for example, since this is a huge change.

I do think that is the only one btw.

@BlackDex
Copy link
Collaborator Author

Also, during this i encountered a few naming-scheme errors, which i probably need to fix too.
Best to do that before merging something like this i think.
Same for some other optimizations like removing the mut for all the DbConn parameters.
Something on my ToDo list :)

@alvinpeters
Copy link

This is great! I'm in the process of adding non-diesel database backends (key-value stores like RocksDB for single-node setup and FoundationDB for clusters). This will make my life much easy. Is there anything I can help (on the coding side) to get this merged?

@BlackDex
Copy link
Collaborator Author

This is great! I'm in the process of adding non-diesel database backends (key-value stores like RocksDB for single-node setup and FoundationDB for clusters). This will make my life much easy. Is there anything I can help (on the coding side) to get this merged?

Not really a.t.m. we were waiting for some other PR's to be merged to prevent big changes for those PR's. Mainly the SSO one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants